fix(link): cancel in-flight <Link> prefetch setup on cache invalidation and scope navigation cancellation to its destination - #2737
Conversation
…on and scope navigation cancellation to its destination A <Link> prefetch whose setup was still awaiting its module imports survived router.refresh(): the invalidation never advanced linkPrefetchNavigationEpoch, so the resumed closure repopulated a navigation-reusable entry from the pre-refresh cache generation. The same global counter also cancelled a pending prefetch for /a when a navigation went to /b, making viewport and intent prefetches timing-dependent for no duplicate-request benefit. Replace the counter with the sticky, destination-scoped pending-setup registry that cloudflare#2709 introduced for router.prefetch(), moved to a dependency-free shims/internal/app-prefetch-setup.ts so link.tsx can register synchronously without pulling navigation.ts onto its startup path. Fixes cloudflare#2718
|
@codex review |
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4e8e71ba7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ld the setup token through staged shell work Codex review on cloudflare#2737 found two gaps in the registry wiring: - The token destination was derived from the basePath-prefixed browser href, which toAppPrefetchDestination prefixes again, so under a non-empty basePath no navigation key ever matched and same-route cancellation silently stopped working. Derive it from the app-relative prefetchHref, matching what navigations pass in. - The outer closure unregistered the token while the fire-and-forget loading shell (and the staged shell-first payload) were still awaiting createRscRequestUrl, so an invalidation in that window found no token and the helper repopulated the invalidated cache. Hold the token until staged work settles and re-check cancellation inside the helpers before their cache writes.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
… boundary Replace the module mock that gated createRscRequestUrl with timing derived from the stubbed fetch itself: a high-priority intent prefetch issues its full-payload fetch synchronously inside the setup closure, so a microtask scheduled from that call lands after the closure returns and before the shell helper's digest-backed createRscRequestUrl resolves. No internal collaborators are mocked.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: LGTM
Reviewed the full diff, ran the affected suites, and traced the cancellation-guard flow against the router.prefetch() reference in navigation.ts. This is a clean, well-motivated refactor and I don't see any correctness issues.
What I verified
Behavior parity with router.prefetch() (#2709). The shared registry (beginPrefetchSetup/cancelPendingPrefetchSetups/finishPrefetchSetup/toAppPrefetchDestination) is a verbatim move out of navigation.ts, and both call sites (notifyAppNavigationStart destination-scoped, invalidatePrefetchCache wholesale via cancelPendingPrefetchSetups(null)) are unchanged. tests/prefetch-cache.test.ts (67) stays green, confirming no router-side regression.
Layering constraint holds. app-prefetch-setup.ts only imports url-utils.js (→ base-path.js) and external-url.js (no imports). No React, no route-trie, no cycle back into navigation.ts/link.tsx. The module can be statically imported on Link's synchronous startup path without pulling in the navigation runtime, as the doc comment claims. Benchmarks confirm zero bundle-size movement.
Cancellation guards cover every cache-write boundary in link.tsx:
- Token registered before the first
await(line 436), re-checked after module loading (462) and after the RSC-URL awaits before the main cache write (554). The main write path (554→788) has no interveningawait, so no cancellation can land in that window — correct. fetchLoadingShellForReusere-checks after its owncreateRscRequestUrlawait (594) before its shell write.- The route-tree gated closure guards only its side write (719) while still resolving the pre-registered navigation entry — the comment at 715-718 correctly explains why.
- The
finallyholds the token viaPromise.allSettled(stagedSetupWork)until the fire-and-forget shell and staged shell-first payload settle, closing the window the first Codex P2 flagged.
basePath token derivation is now correct. The Link token uses toAppPrefetchDestination(prefetchHref) (app-relative), matching router.prefetch()'s toAppPrefetchDestination(prefetchHref) at navigation.ts:2561/2576, not the basePath-prefixed fullHref. This resolves the second Codex P2. The ?? fullHref fallback is genuinely unreachable: getLinkPrefetchHref already returns null for cross-origin hrefs, and the code runs client-side inside hasAppNavigationRuntime(), so toAppPrefetchDestination never returns null here.
Behavior-change surface is sound. Reverting notifyLinkNavigationStart to not cancel (dropping the global epoch) is the whole point — raw history.pushState no longer drops unrelated pending prefetches, matching the router-side rationale in notifyAppNavigationStart. No dangling references to linkPrefetchNavigationEpoch/navigationEpoch remain.
Checks
vitest run tests/link-navigation.test.ts→ 79 passedvitest run tests/prefetch-cache.test.ts→ 67 passedvp linton all three source files → clean
The five new regression tests each pin a distinct invariant (stale resume across invalidation, same-route vs. elsewhere scoping as a pair, basePath key derivation, staged-shell leak) and the PR notes they were verified to fail on the pre-change implementation. Nice work.
|
Review posted successfully. Here's my summary: Review complete — LGTMI reviewed PR #2737 (fork, review-only mode), read all four changed files, traced the cancellation flow against the Key findingsThe refactor is sound. The prefetch-setup registry is a verbatim move from Layering constraint holds. The new module imports only Cancellation guards cover every cache-write boundary in Both Codex P2 issues were properly fixed:
The Verification
|
2 participants
Overview
<Link>prefetch setup the same cancellation semanticsrouter.prefetch()gained in #2709linkPrefetchNavigationEpochwith the sticky, destination-scoped pending-setup registry, moved to a shared dependency-free moduleshims/internal/app-prefetch-setup.tsowns registration and cancellation;navigation.tsowns the policy of when to cancelrouter.refresh()can no longer be undone by a late<Link>prefetch; navigations no longer drop unrelated pending prefetchesCloses #2718.
Why
A prefetch whose asynchronous setup is still in flight must not commit state from a cache generation that has since been invalidated, and a navigation should only cancel prefetch setup it is about to duplicate. #2709 built both guarantees for
router.prefetch()as a registry of sticky per-destination tokens, but<Link>'sprefetchUrlkept its own global counter with neither property:invalidatePrefetchCache()voids the whole generation, so every pending setup must die with it<Link>setup awaiting its lazy module imports resumed afterrouter.refresh()and repopulated a navigation-reusable entry from the pre-refresh generation/bsilently dropped a pending prefetch for/athat nothing else would ever fetchlink.tsxloadsnavigation.tslazily to keep the navigation runtime off Link's synchronous startup pathshims/internal/module both files import statically, following the layeringapp-route-prefetch-policy.tsestablished in #2709Sticky per-token cancellation is what makes destination scoping sound: a navigation to
/afollowed by one to/bleaves a pending/aprefetch cancelled, where comparing against a "current destination" value would not.What changed
router.refresh()while a<Link>prefetch awaits its module imports<Link>prefetch<Link>prefetch is pendinghistory.pushState(no request issued)<Link>prefetchesnotifyAppNavigationStartrouter.prefetch()(all #2709 scenarios)navigation.tstests/prefetch-cache.test.tsunchanged and greenMaintainer review path
packages/vinext/src/shims/internal/app-prefetch-setup.tsfor the moved registry and the layering constraint its doc comment states (why it must stay dependency-free).packages/vinext/src/shims/navigation.tsfor the mechanical switch to imports; cancellation policy call sites (notifyAppNavigationStart,invalidatePrefetchCache) are unchanged.packages/vinext/src/shims/link.tsxfor the substantive change:prefetchUrlregisters a token synchronously before its firstawait, re-checkscancelledafter the module-loading race and again after the RSC-URL awaits before any cache write, and unregisters infinally. The navigation-runtime hook reverts to plainnotifyLinkNavigationStartsince cancellation no longer rides it.tests/link-navigation.test.tsfor the three regression shapes.Validation
prefetchUrlvia therenderIsolatedLinkharness:vp test rungreen on the three directly affected suites (263 tests) and on the 17 suites importing the navigation or link shims (1,986 tests);vp checkclean on all changed files; pre-commit full check and knip passed.Risk / compatibility
shims/internal/and the removed counter was module-private.url-utils,utils/external-url) were already on Link's synchronous path.<Link>prefetches for other destinations. This trades a possible duplicate request in zero scenarios (nothing else fetches those routes) for prefetch reliability, matching the tradeoff already accepted forrouter.prefetch()in fix(navigation): reuse router.prefetch payloads during navigation #2709.References
router.prefetch(); its doc comment deferred this unificationtests/prefetch-cache.test.ts